@stavros11 and @hay-k I asked you a review immediately, such that you could comment on the overall strategy before I keep going to apply it to the whole file.

The new part is down to class Shapes, everything else still depends on the old PulseShape, and I haven't touched yet. But there are no surprises, it will be pretty straightforward (and it will collapse the number of lines in that file).

@stavros11 and @hay-k, feature-wise the PR should be complete, now I need to fix the tests and propagate the changes.

If you'd start having a look you could already provide some feedback.

Question: is there any case where times is not something like np.linspace(0, duration, nsamples) (where nsamples = sampling_rate * duration))? Is this array supposed to be generated in all drivers calling the pulse.i and pulse.q methods?

Thanks for asking: the honest answer is "I don't know". However, here I started with the assumption that I wanted to evaluate a function on some points. Then, I had to change my mind.
And, in the last reshuffle, I even assumed a linspace, to use the scipy.signal.windows.gaussian. So, we should definitely decide to either assume a linspace everywhere (and possibly retain only the linspace parameters), or I should revert the usage of the SciPy function.

The other point is about the way we are handling i and q, which seems to me as kind of reinventing complex numbers. Since we are using numpy, which supports complex numbers,

Concerning this, I have little experience with the drivers, so I don't know whether they are going to consume this information separately, or as complex numbers.

In any case, I believe it is valuable to keep .i() and .q() in Envelope, since they are usually defined as two independent real functions, rather than a single real-to-complex one. But we could collect them in a single complex number, and expose only that at the level of the Pulse (i.e. drop Pulse.i()/.q() and keep only Pulse.envelopes(), possibly renaming it - and drop Envelope.envelopes() as well, since it is duplicated in Pulse, and definitely not required at low level).

Concerning this, I have little experience with the drivers, so I don't know whether they are going to consume this information separately, or as complex numbers.

Probably most instruments instruments do not support uploading complex waveforms (but maybe ZI does?), but even in that case I would be fine doing

waveform = pulse.envelope(...)
upload_to_instrument...({"i": waveform.real, "q": waveform.imag})

over

upload_to_instrument...({"i": pulse.i(...), "q": pulse.q(...)})

if the former helps simplifying other parts of qibolab.

waveform = pulse.envelopes(...)
upload_to_instrument...({"i": waveform[0], "q": waveform[1]})

This is already possible. We could follow PEP 20 and drop an alternative.

However, I'm not sure whether a shape=(n,), dtype=complex array is any better than a shape=(2,n), dtype=float in this case.

However, I'm not sure whether a shape=(n,), dtype=complex array is any better than a shape=(2,n), dtype=float in this case.

Yes, I am aware of pulse.envelopes(...) and I am also not sure about which one is better (they may even be equivalent).
The main advantage of complex is that it can simplify some manipulations we are doing, such as the modulation/demodulation functions (I think we can get rid of einsum if we use complex) or the transformation in my comment above about the phase (if we decide to implement it). Ideally, we would also like to be consistent with the results and use real or complex in both places.

@stavros11 usually complex numbers are useful in electronics, because it's simpler to integrate/derive exponentials, and it's easier to compose them (multiplication would just sum the exponents).
But I'm not sure we would leverage any of these niceties in our code, and if we're just working with real and complex parts separately, it's just simpler to use two arrays (concerning mod/demod you are right, since sine and cosine could be combined in a single exponential, so we'd get rid of one dimension - but it's only relevant for software modulation, and I'd say that is already simple enough...)

@hay-k @stavros11: to me, the current layout of pulses.shape is final. I'm now tempted to just fix the tests and merge (of course I will ask you to review after fixing the tests).

If you have any further comment about the strategy, now it's the best timing :)

I applied @stavros11 proposal concerning linspace, and I replied above about complex and relative phases. I'm not aware of anything else related to this subject, yet.

I believe I achieved a new record:

========================================== 2103 failed, 126 passed, 94 skipped, 47 deselected, 3 xfailed, 3 xpassed, 3 warnings in 108.28s (0:01:48) ==========================================

(i.e. everyone depended on PulseShape, ...)

I'm going to fix the tests in a quick and dirty way (the least dirty the best, of course). The message is: I'll delegate tests improvements to everyone that is going to improve the corresponding source, as more competent on the matter (I believe is a sensible attitude for every PR, since the goal is minimizing the changes).

If anyone at any time (mainly @stavros11 and @hay-k) has a chance to decouple some tests from the pulses subpackage, please do your best. Otherwise, further improvements will always be a mess...

Instead, I'm going to take care of shapes-specific tests.

I'm actually keeping the same signature as before for pulses envelopes, since sampling_rate is the only information missing to a pulse to determine which samples to take. And it is truly external, so it should be provided just for the envelope computation (instead of stored by the pulse, since the exact same pulse would allow different sampling rates).

However:

• I'm not applying any default for the sampling rate, since there is no default machine (dummy can define its own, as it is doing)
• the envelopes will still ask for the Times (i.e. duration and number of samples), since that's the required information - the Pulse can generate it given the sampling rate

Codecov Report

Attention: Patch coverage is 89.92537% with 27 lines in your changes are missing coverage. Please review.

Project coverage is 48.75%. Comparing base (6790872) to head (6f4c84b).

@@            Coverage Diff             @@
##              0.2     #818      +/-   ##
==========================================
- Coverage   49.52%   48.75%   -0.77%     
==========================================
  Files          59       61       +2     
  Lines        5642     5532     -110     
==========================================
- Hits         2794     2697      -97     
+ Misses       2848     2835      -13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Just rebased. I will merge as soon as the tests pass.